Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REF] account: Add test for issue#30972 #31480

Conversation

keylor2906
Copy link
Contributor

@keylor2906 keylor2906 commented Feb 28, 2019

Description of the issue/feature this PR addresses:
Avoid the creation of a cash basis entry when reversing an invoice journal entry.

Steps to reproduce:
Previous configurations:

  1. Active cash basis on the accounting settings
  2. Configure the Tax Due = Based on Payment on the purchase tax
  3. Configure the tax account as Allow Reconciliation
  4. Generate a new supplier invoice with the same tax configured.
  5. Validate the invoice
  6. Go to the journal entry on the invoice and try to generate a Reverse Entry

Current behavior before PR:
A cash basis journal entry is created when reversing the invoice journal entry.
And an error message You are trying to reconcile some entries that are already reconciled. is returned when trying to generate the reversal.

Desired behavior after PR is merged:
Not create cash basis journal entry when reversing the invoice journal entry. The cash basis entries should be only created on payments. And not getting error message when trying to generate the reversal.

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@moylop260
Copy link
Contributor

Unittest for case #30972

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Feb 28, 2019
@moylop260
Copy link
Contributor

@hbto @luistorresm
What do you think?

@hbto
Copy link
Contributor

hbto commented Feb 28, 2019

@moylop260 @keylor2906

Test must be done for both Supplier Invoices & Customer Invoices,

I can see that the test is done for one of them only and

this piece of code is suspicious

cash_basis = debit_moves and debit_moves[0].account_id.internal_type in ('receivable', 'payable') and **not debit_moves[0].invoice_id** or False

because it is considering only one side for the invoices (debit_moves)

@keylor2906 keylor2906 force-pushed the 12.0-add-test-revert-invoice-dev-keylor2906 branch from 118ec43 to 75d9ec8 Compare February 28, 2019 21:32
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Feb 28, 2019
@keylor2906
Copy link
Contributor Author

@hbto Added the test for Customer Invoices.

Regards.

@keylor2906 keylor2906 force-pushed the 12.0-add-test-revert-invoice-dev-keylor2906 branch from 75d9ec8 to 9027c9f Compare March 1, 2019 21:10
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Mar 1, 2019
@moylop260
Copy link
Contributor

@JulioSerna
Could you check it, please?

@@ -823,7 +823,7 @@ def _reconcile_lines(self, debit_moves, credit_moves, field):
"""
(debit_moves + credit_moves).read([field])
to_create = []
cash_basis = debit_moves and debit_moves[0].account_id.internal_type in ('receivable', 'payable') or False
cash_basis = debit_moves and debit_moves[0].account_id.internal_type in ('receivable', 'payable') and not debit_moves[0].invoice_id or False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why only the debit_move and not the credit_move?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it work for both the in_invoice and the out_invoice?

it is quite weird for me.

Regards

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being honest, I don't really know why the debit_moves and not the credit_moves, I only did the change based on the previous criteria for creating a cash basis move (debit_moves) and works for both cases, in and out invoices.

@hbto
Copy link
Contributor

hbto commented Mar 14, 2019

Other than that comment I am ok with this PR

…ice account.move when the tax cash basis account has set reconcile True.

[REF] account: Not create cash basis move when reversing an invoice
@keylor2906 keylor2906 force-pushed the 12.0-add-test-revert-invoice-dev-keylor2906 branch from 9027c9f to 35f2e50 Compare March 15, 2019 00:54
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Mar 15, 2019
@hbto
Copy link
Contributor

hbto commented Mar 15, 2019

👍 given the explanation of @keylor2906 and based on the fact that this commit has the regarding unit tests that support it, I agree on this PR.

Regards

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Mar 15, 2019
@JulioSerna
Copy link
Contributor

LGTM 👍

@moylop260
Copy link
Contributor

@qdp-odoo @smetl
May I ask you to check this PR, Please?

@kebeclibre
Copy link
Contributor

@moylop260
Check out my PR, I use a context key, which I don't like to do at all, especially in accounting.
But it has the advantage of:

  • being accurate: we really are in a context were no cash basis is necessary since the is no cash flow
  • working with any move, even manual ones

Your fix is smart, but I'm afraid it will work only for invoice moves and not for any random move

@pedrobaeza
Copy link
Collaborator

coming to this through codetriage.com. @moylop260 any answer or can this be closed?

@moylop260
Copy link
Contributor

@hbto
Is it valid yet?

@hbto
Copy link
Contributor

hbto commented Apr 25, 2020

Yes @moylop260 this is still valid.

Regards

@C3POdoo
Copy link
Contributor

C3POdoo commented Apr 22, 2022

Dear @keylor2906,

Thank you for your contribution but the version 12.0 is no longer supported.
We only support the last 3 stable versions so no longer accepts patches into this branch.

We apology if we could not look at your request in time.
If the contribution still makes sense for the upper version, please let us know and do not hesitate to recreate one for the recent versions. We will try to check it as soon as possible.

This is an automated message.

@C3POdoo C3POdoo closed this Apr 22, 2022
@luisg123v luisg123v deleted the 12.0-add-test-revert-invoice-dev-keylor2906 branch July 14, 2022 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accounting CI 🤖 Robodoo has seen passing statuses
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants